Skip to content

Fallback to current Feishu message for reactions#496

Open
zhulijin1991 wants to merge 3 commits into
larksuite:mainfrom
zhulijin1991:codex/lark-reaction-defaults
Open

Fallback to current Feishu message for reactions#496
zhulijin1991 wants to merge 3 commits into
larksuite:mainfrom
zhulijin1991:codex/lark-reaction-defaults

Conversation

@zhulijin1991
Copy link
Copy Markdown
Contributor

@zhulijin1991 zhulijin1991 commented May 11, 2026

Summary

  • Keep this PR focused on reaction message-id resolution only.
  • Let react / reactions fall back to toolContext.currentMessageId when callers omit messageId / message_id.
  • Remove the runtime emoji alias map from this branch so canonical Feishu reaction names are never silently rewritten.

Validation

  • pnpm exec vitest run tests/reaction-defaults.test.ts
  • pnpm run typecheck
  • pnpm exec prettier --check src/messaging/outbound/actions.ts tests/reaction-defaults.test.ts
  • git diff --check

Follow-up

Emoji-shape guidance should be handled separately in tool schema/description wording, without hidden runtime rewrites such as HEART -> LOVE or CLAP -> APPLAUSE.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 11, 2026

CLA assistant check
All committers have signed the CLA.

Copy link
Copy Markdown
Collaborator

@evandance evandance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — the friction this addresses is real (agents do try react({ emoji: '👍' }) first). Two changes here with very different risk profiles; please split.

readReactionMessageId — ready to merge (after split). Falling back to toolContext.currentMessageId is clean. SDK type checks out, precedence is right (messageId > message_id > fallback), tests cover the three branches.

Alias map — silent regression that CI didn't catch. Two entries in FEISHU_REACTION_ALIASES collide with canonical Feishu types that are already valid and in use:

['clap',  'APPLAUSE']   // `CLAP` ≠ `APPLAUSE` in Feishu (both valid, different emojis)
['heart', 'LOVE']       // `HEART` ≠ `LOVE`     in Feishu (both valid, different emojis)

The case-insensitive lookup (MAP.get(normalized) ?? MAP.get(normalized.toLowerCase()) ?? normalized) silently rewrites canonical uppercase type names at runtime:

normalizeReactionEmoji('HEART') -> 'LOVE'      ⚠️
normalizeReactionEmoji('CLAP')  -> 'APPLAUSE'  ⚠️

There's no existing test exercising 'HEART' or 'CLAP' through normalizeReactionEmoji. The new 'MUSCLE' passthrough test only works because there's no muscle collision. So CI stays green even though any agent passing FeishuEmoji.HEART or FeishuEmoji.CLAP (canonical names already exported from src/messaging/outbound/reactions.ts) would post a different emoji after this merges.

Recommended fix: move aliasing to the tool schema, not code. The reason agents pass '👍' is the tool's emoji field description doesn't tell them what shape to use. The right place to teach the model is the schema, not a runtime alias map. It teaches once (the model sees the schema on every tool call and learns the canonical shape upfront, no validation-error round-trip), stays in sync automatically (VALID_FEISHU_EMOJI_TYPES is the authoritative list sourced from official Feishu docs and the schema description can reference it directly), keeps the typed contract honest (type stored = type passed, no hidden rewrites for future readers to chase), and rules out this regression class entirely (schema documentation can't silently rewrite caller input). Concretely, extend the emoji field description with something like "Feishu reaction type name (e.g. THUMBSUP, LAUGH, HEART, LOVE). See the full list in VALID_FEISHU_EMOJI_TYPES. Emoji unicode like 👍/+1 is not accepted — use the canonical type name."

Fallback (only if schema-layer isn't enough). If you have a trace showing the model keeps passing '👍' even after the description is updated, code-layer aliasing becomes justified. In that case, scope it to emoji unicode → canonical type only (drop word aliases like thumbsup/heart/clap — those collide) and gate on "not already in VALID_FEISHU_EMOJI_TYPES" so that if a value is already canonical it returns as-is and skips the map. This eliminates the regression class entirely and makes the contract explicit ("we only translate emoji unicode, never canonical names").

Action items. First, split this PR — readReactionMessageId + its 3 tests can ship as a standalone PR, and I'll approve and merge it as soon as the split lands. Second, for the alias half, update the tool schema description first; reopen with code-layer aliasing only if there's evidence schema docs aren't sufficient.

@evandance evandance added feature request New feature or request messaging src/messaging/ + src/card/ — message rendering, cards, streaming changes requested Need do changes labels May 12, 2026
@zhulijin1991
Copy link
Copy Markdown
Contributor Author

Thanks, this is a good catch. I agree the two parts should be split, and the HEART / CLAP collisions make the runtime alias map unsafe as written.

I will keep the readReactionMessageId fallback as the focused mergeable change, then move the emoji-shape guidance to the tool schema instead of rewriting word aliases at runtime. If we later get a real trace showing unicode emoji still appears after the schema description is tightened, I will reopen that as a narrower unicode-only translation guarded by canonical passthrough.

@zhulijin1991 zhulijin1991 changed the title Normalize Feishu reaction defaults Fallback to current Feishu message for reactions May 13, 2026
@zhulijin1991
Copy link
Copy Markdown
Contributor Author

The branch is now narrowed to the mergeable readReactionMessageId fallback only (head b000586).

The runtime emoji alias map from the earlier review commit is no longer in this PR, so canonical HEART / CLAP are not rewritten. The remaining diff is the current-message fallback plus its three tests.

Validation already run locally:

  • pnpm test tests/reaction-defaults.test.ts
  • pnpm typecheck

Ready for re-review.

@zhulijin1991 zhulijin1991 force-pushed the codex/lark-reaction-defaults branch from b000586 to c3d1513 Compare May 17, 2026 07:59
@zhulijin1991
Copy link
Copy Markdown
Contributor Author

Updated in c3d1513.\n\nChanges:\n- Rebased onto current upstream/main and resolved the conflict in src/messaging/outbound/actions.ts.\n- Kept the branch narrowed to readReactionMessageId fallback only: explicit messageId > message_id > toolContext.currentMessageId.\n- Confirmed the runtime emoji alias map is not present in the final diff.\n\nValidation:\n- pnpm test tests/reaction-defaults.test.ts\n- pnpm typecheck\n\nReady for re-review.

Copy link
Copy Markdown
Collaborator

@evandance evandance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reading this against the SDK surface — openclaw/plugin-sdk/channel-actions already exports resolveReactionMessageId, which does readStringOrNumberParam(args, 'messageId') ?? toolContext?.currentMessageId. The SDK's own Discord and Signal channel adapters use it directly; Feishu is the outlier. The self-rolled readReactionMessageId here both duplicates the SDK and drifts from it (string-only vs string|number).

Note the SDK reader already accepts snake_case automatically — readStringOrNumberParam(args, 'messageId') will pick up args.message_id via readSnakeCaseParamRaw. So the explicit message_id branch and its test add no behavior over the SDK helper.

Please switch handleReact / handleReactions to the SDK helper, match the Discord/Signal pattern (String(...) cast, throw with "messageId required. Provide messageId explicitly or react to the current inbound message."), and drop tests/reaction-defaults.test.ts — the helper is owned and tested by the SDK now.

@zhulijin1991
Copy link
Copy Markdown
Contributor Author

Updated in e918da9.\n\nChanges:\n- Switched Feishu react/reactions message ID resolution to the SDK resolveReactionMessageId helper from openclaw/plugin-sdk/channel-actions.\n- Matched the existing channel pattern by casting the resolved string|number with String(...).\n- Replaced the missing-message error with: "messageId required. Provide messageId explicitly or react to the current inbound message."\n- Dropped tests/reaction-defaults.test.ts because the helper behavior is owned by the SDK.\n\nValidation:\n- pnpm lint\n- pnpm format:check\n- pnpm typecheck\n- pnpm test\n\nReady for re-review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes requested Need do changes feature request New feature or request messaging src/messaging/ + src/card/ — message rendering, cards, streaming

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants